1376119 - containers: "reopen in .." option

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

I realize there are good reasons not to move a tab between containers, but in most cases just re-opening the tab in a new container would be sufficient.  For example, if I open a github link in the default container rather than in my "dev" container, I'll notice that github does not show me logged in.

The current steps to fix this are, copy the URL, click a few times to open a new tab in the dev container, and paste the URL there.

This feature would just shortcut those steps: click -> reopen page as.. -> dev

Comment 1

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 2

[Tooru Fujisawa [:arai]]

I really need this.
taking.

Comment 3

[Tooru Fujisawa [:arai]]

We might need to think about how this operation will be observed from WebExtensions API.
Just opening new tab with specified container and closing current tab will be fine per API, but a bit ugly.
If we change the container in place, the API might need to be updated to notify the change (I don't see cookieStoreId in tabs.update/onUpdated).

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Add Reopen in Container tab menu.

Here's naive implementation that really reopens the tab, carrying over the following information:
  * position
  * selected
  * muted
  (is there any other thing needs to be carried over?)
and close the original tab.

How do you think about this approach?

Should we reuse the tab/browser instead of creating new one?
in that case WebExtensions API should be updated I think.

Comment 5

[Jonathan Kingston [:jkt] he/him]

We explicitly have not done this, not because it is hard just because we are not wanting to push this path.

Tanvi should this be blocked by UX or clearing history options as we have mentioned before?

> Should we reuse the tab/browser instead of creating new one?

I'm pretty sure you can't do this currently if I understand you correctly.

Comment 6

[Tanvi Vyas[:tanvi]]

Can you provide a screenshot of what the UI looks like from your patch?

See https://github.com/mozilla/multi-account-containers/issues/942 where we are trying to solve this problem in the Multi-Account Containers addon rather than the platform.

The current tab should not close automatically.  We want to provide a "reopen" option, not a "switch container type" option.  In the addon, we can achieve this by putting something in the addon created browser popup menu.  If we were to do this in the platform, I would image using the url bar and (left/right?) clicking on the name/icon of the current container.  That could have a drop down that looked like:

Reopen in...
  Personal
  Work
  Shopping
  etc.

The user could select a Container and a new tab could open with the same url but a different Container type.  (I imagine we should also support the default container or "No Container" option.  The concept of the default Container could be a little confusing.)

This needs a user experience designer.  They can review the above proposal and make modifications as they see fit.  For now, I think we should put it into Multi-Account Containers as described in the github issue and see how much it is used.  Putting it in the extension rather than platform does require a few more clicks (3 clicks instead of 2?), but its worth doing it there first before introducing it to platform.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: screenshot of the menu

here's screenshot.
(clearly the 1st one is wrong that it shows "No Container" :P )

I'll fix the patch to just reopen without closing

Comment 8

[Tanvi Vyas[:tanvi]]

Please make sure the tab menu item for "Reopen in Container" only appears when the about:config preference privacy.userContext.enabled is set to true.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Add Reopen in Container tab menu.

Now the menu just reopens the same URL in the next tab, without closing the current tab.
and also fixed the "No Container" visibility.

Comment 10

[Tooru Fujisawa [:arai]]

looks like almost same feature can be implemented in standalone webextension, in the same tab menu (but the menu item of the current container cannot be hidden).
I'll look into the Multi-Account Containers later.

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> Created attachment 8939229 [details] [diff] [review]
> Add Reopen in Container tab menu.
> 
> Now the menu just reopens the same URL in the next tab, without closing the
> current tab.
> and also fixed the "No Container" visibility.

Tooru, can you provide an updated screenshot?

Comment 12

[Tanvi Vyas[:tanvi]]

Comment on attachment 8939229 [details] [diff] [review]
Add Reopen in Container tab menu.

baku or jkt - can one of you take a look at this?

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: screenshot of the menu

(In reply to Tanvi Vyas[:tanvi] from comment #11)
> Tooru, can you provide an updated screenshot?

sure

Comment 14

[Jonathan Kingston [:jkt] he/him]

f+ from me if you remove `isReopenMenu` and just use `isContextMenu`. This likely will need a rebase too.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Add Reopen in Container tab menu.

Thank you for your feedback :)

removed isReopenMenu and rebased.

I'll add similar feature to Multi-Account Containers shortly.

Comment 16

[Tooru Fujisawa [:arai]]

here's WIP patch for Multi-Account Containers:
https://github.com/arai-a/multi-account-containers/commit/d328638b10e2ebc18325b0fea1dd68b5c68452af

The difference is:
  * the menu item is shown at the botton
  * the menu item for current container is disabled, instead of hidden
  * the container icon's color is not reflected

Comment 17

[Tooru Fujisawa [:arai]]

opened https://github.com/mozilla/multi-account-containers/pull/1215

(In reply to Tooru Fujisawa [:arai] from comment #16)
> here's WIP patch for Multi-Account Containers:
> https://github.com/arai-a/multi-account-containers/commit/
> d328638b10e2ebc18325b0fea1dd68b5c68452af
> 
> The difference is:
>   * the menu item for current container is disabled, instead of hidden

this one is fixed.
(now the menu items are rebuilt every time, for simplicity and as a workaround for initial run)

Comment 18

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8974290 [details] [diff] [review]
Add Reopen in Container tab menu.

Are you able to review this one Baku? It seems fine to me, we should take it over  the MAC addon change IMO.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8974290 [details] [diff] [review]
Add Reopen in Container tab menu.

Review of attachment 8974290 [details] [diff] [review]:
-----------------------------------------------------------------

I cannot give a r+ on browser code. Gijs, can you take a look?

Comment 20

[:Gijs (he/him)]

Comment on attachment 8974290 [details] [diff] [review]
Add Reopen in Container tab menu.

Review of attachment 8974290 [details] [diff] [review]:
-----------------------------------------------------------------

A few notes below. I don't really understand why we want to take this in Firefox rather than the add-on, but if we're convinced this needs to happen in-browser, the code looks good to me besides the comments below. :jkt, can you comment?

It would be nice to have a test though, and I'm not sure about what accesskey to use instead, so holding off on r+ to get that sorted out.

::: browser/base/content/browser.js
@@ +4510,5 @@
>  }
>  
>  /**
>   * Updates File Menu User Context UI visibility depending on
>   * privacy.userContext.enabled pref state.

Please update the comment.

@@ +4578,5 @@
> +  // Carry over some configuration.
> +  if (isSelected) {
> +    gBrowser.selectedTab = newTab;
> +  }
> +  if (currentTab.pinned) {

Pass this as a param in options instead.

@@ +4586,5 @@
> +    if (!newTab.muted) {
> +      newTab.toggleMuteAudio(currentTab.muteReason);
> +    }
> +  }
> +  gBrowser.moveTabTo(newTab, currentTab._tPos + 1);

Instead, please pass the index as part of the options you pass to `addTab`.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +54,5 @@
>  <!ENTITY  sendToDeviceOfflineFeedback.label  "Queued (offline)">
>  <!ENTITY  moveToNewWindow.label              "Move to New Window">
>  <!ENTITY  moveToNewWindow.accesskey          "W">
> +<!ENTITY  reopenInContainer.label            "Reopen in Container">
> +<!ENTITY  reopenInContainer.accesskey        "C">

This access key conflicts with 'close tab', as far as I can tell?

Comment 21

[Jonathan Kingston [:jkt] he/him]

> I don't really understand why we want to take this in Firefox rather than the add-on, but if we're convinced this needs to happen in-browser

Mostly that we think this is a core feature for anyone using containers and not just specific to the addon itself.

:tanvi, is there anything to add here? You asked me to provide feedback on this a while ago.

Comment 22

[Tanvi Vyas[:tanvi]]

The re-open in option solves a lot of usability issues when you find you are in the wrong container and hence not logged in.  You have to open another tab in the right container, copy/paste the link, and sometimes it automatically takes you to back to the original tab with "Switch to Tab".  Baking the re-open option right into the context menu shortcuts this.

From a UX perspective, it might fit nicely as a right click on the Container name in the url bar itself, but that is a bigger UI change.  This one is minor, and should only show up for users who have Containers turned on.

Comment 23

[Tooru Fujisawa [:arai]]

One more reason to implement this as built-in feature:
https://github.com/mozilla/multi-account-containers/pull/1215#discussion_r190748229

WebExtension cannot open certain URLs due to security reason, so we should either open other URL for the case or hide the menu item for such tab.
If we implement this as built-in feature, such issue doesn't happen.

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Add Reopen in Container tab menu.

* Updated comments
  * Fixed the addTab callsite to pass more info as parameter
  * Changed accesskey to "e", which seems not to conflict
  * Added testcase
  * Separate updateUserContextUIVisibility into updateFileMenuUserContextUIVisibility and updateTabMenuUserContextUIVisibility,
    given that the File menu item is disabled in Private Window,
    but Tab menu item should be hidden

Comment 25

[:Gijs (he/him)]

I think we're good to add this to core, then. :arai, is the new patch ready for review, or does it need more work?

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Add Reopen in Container tab menu.

Thanks!
here's updated patch (fixed testcase to pass lint)

Comment 27

[:Gijs (he/him)]

Comment on attachment 8983389 [details] [diff] [review]
Add Reopen in Container tab menu.

Review of attachment 8983389 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +4564,5 @@
>  }
> +function updateTabMenuUserContextUIVisibility(id) {
> +  let menu = document.getElementById(id);
> +  // Visibility of Tab menu item can change frequently.
> +  menu.hidden = !Services.prefs.getBoolPref("privacy.userContext.enabled") ||

nit: please add a default to the getBoolPref call (probably false)

Comment 28

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36ced9cc9edaf4600e48d1707755851fa0531b48
Bug 1376119 - Add Reopen in Container tab menu. r=baku,Gijs

Comment 29

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/36ced9cc9eda

Comment 30

[Pascal Chevrel:pascalc]

Tooru, could you do a release notes addition request please? We want this mentionned in Nightly release notes at least, thanks. https://wiki.mozilla.org/Release_Management/Release_Notes#How_to_nominate_a_bug_for_release_notes_addition.3F

Comment 31

[Tooru Fujisawa [:arai]]

Release Note Request (optional, but appreciated)

> [Why is this notable]

as mentioned in comment #22, this fix helps the flow when one noticed that they
opened a tab in a wrong container, which often happens, when one opens the link from external application, or maybe bookmark, or in some other ways.
now they can open the page in the right container just by choosing the tab menu item, instead of copy-paste-ing the URL.

> [Affects Firefox for Android]

No

> [Suggested wording]

(not sure if this explains well, please fix the wording :)
Added "Reopen in Container" tab menu item, which allows you to open the tab in the right container when it's accidentally opened in an unexpected container.

> [Links (documentation, blog post, etc)]

None

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

This is in Nightly 62 release notes -- Is this general containers pref turned on for dev edition also, or just for nightly?

Comment 33

[Tooru Fujisawa [:arai]]

this feature doesn't have specific pref, but just depends on privacy.userContext.enabled which is enabled depending on NIGHTLY_BUILD flag, so, by default this is nightly-only.
but can be enabled by pref, and also by installing Multi Account Container extension [1] which can be installed on Release as well

[1] https://addons.mozilla.org/en-US/firefox/addon/multi-account-containers/

Comment 34

[Colin Devroe]

I'm so looking forward to using this. Please forgive my ignorance, I'm running FF 60.0.2 with Containers 6.0.0. Can I enable this somehow today? Or, will I need to run a Nightly FF build?

Comment 35

[Tooru Fujisawa [:arai]]

this is fixed in 62, which is currently Nightly and will be released on September

Comment 36

[Colin Devroe]

(In reply to Tooru Fujisawa [:arai] from comment #35)
> this is fixed in 62, which is currently Nightly and will be released on
> September

Thank you.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Firefox 62 release notes.